Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify configuration methods #551

Merged
merged 12 commits into from
Feb 2, 2024
Merged

Unify configuration methods #551

merged 12 commits into from
Feb 2, 2024

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jan 17, 2024 •
edited
Loading

  • Checks for config file in:

    • <current_folder>/.config/espflash.toml
      • Not sure if this is the best path. Maybe other option could be reusing .cargo/config.toml as its present in most(or all) our project.
    • <config_dir>/espflash/espflash.toml

  • Supports the following configurations:

    • Preferred serial and usb connections
      • If we set the ESPFLASH_PORT it will be used instead of the config file value
    • Bootloader
    • Partition table
    • Baudrate
      • If we set the ESPFLASH_BAUD it will be used instead of the config file value

  • [cargo-espflash]: We no longer support setting bootloader, format and partition table in cargo metadata

@SergioGasquez SergioGasquez linked an issue Jan 17, 2024 that may be closed by this pull request
RFC: Rework configuration methods #507
Closed
@SergioGasquez SergioGasquez linked an issue Jan 31, 2024 that may be closed by this pull request
Attempt to read espflash config from project_dir/.config/espflash.toml #469
Closed
@SergioGasquez SergioGasquez marked this pull request as ready for review February 1, 2024 10:48
@MabezDev
Copy link
Member

MabezDev commented Feb 1, 2024

<current_folder>/.config/espflash.toml

What's the rationale around having it inside <current_dir>/.config, instead of <current_dir>? It doesn't matter much to me, but I just wonder if there is a particular reason. Most other tools with configs, namely probe-rs (cargo embed) place there configuration files in the root of the project.

MabezDev
MabezDev reviewed Feb 1, 2024
View reviewed changes
cargo-espflash/src/package_metadata.rs Outdated
.get("bootloader")
.map(|bl| package.root().join(bl.as_str().unwrap()));

// TO BE REMOVED?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remind me again what this is? I don't use cargo-espflash very often 😅.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could set the partition-table, bootloader and format in the package metadata, see https://github.com/esp-rs/espflash/tree/main/cargo-espflash#package-metadata. At the moment, I removed baudrate and partition-table properties. IMHO, we can also remove the format

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, thanks! Yes lets remove format too.

@SergioGasquez
Copy link
Member Author

What's the rationale around having it inside <current_dir>/.config, instead of <current_dir>? It doesn't matter much to me, but I just wonder if there is a particular reason. Most other tools with configs, namely probe-rs (cargo embed) place there configuration files in the root of the project.

Tbh, I just followed the suggestion in #469, I can update the path to be <current_dir>

@MabezDev
Copy link
Member

MabezDev commented Feb 1, 2024

imo it should probably be in the current directory, but I'll let @jessebraham @bjoernQ and @JurajSadel weigh in too.

@SergioGasquez
Copy link
Member Author

imo it should probably be in the current directory, but I'll let @jessebraham @bjoernQ and @JurajSadel weigh in too.

Awesome, let's hear their opinion, I am happy to change it with whatever we decide

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 1, 2024

I think I would prefer the current directory

@jessebraham
Copy link
Member

Sorry if I've missed something, why was the Cargo package metadata loading in cargo-espflash crippled? What's the point of even having it in its current rendition?

@SergioGasquez
Copy link
Member Author

Sorry if I've missed something, why was the Cargo package metadata loading in cargo-espflash crippled? What's the point of even having it in its current rendition?

I'd say we can remove the Cargo metadata, as it is now in the config file and its the same for espflash/cargo-espflash

@jessebraham
Copy link
Member

That's disappointing, I use it in most of my projects :/ Guess I will switch over, thanks.

@SergioGasquez
Copy link
Member Author

That's disappointing, I use it in most of my projects :/ Guess I will switch over, thanks.

I mean, we can keep it. No hard opinion there, but then we would have multiple ways of defining those configs.

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Feb 2, 2024 •
edited
Loading

I updated the local config to live under <current_dir>/espflash.toml, removed the format param from cargo metadata for cargo-espflash and udpated the docs

bjoernQ
bjoernQ approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

JurajSadel
JurajSadel approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SergioGasquez SergioGasquez merged commit a55179f into esp-rs:main Feb 2, 2024
20 checks passed
@SergioGasquez SergioGasquez deleted the feat/config branch February 2, 2024 10:37
@marsfan marsfan mentioned this pull request Feb 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MabezDev MabezDev MabezDev left review comments

@JurajSadel JurajSadel JurajSadel approved these changes

@bjoernQ bjoernQ bjoernQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC: Rework configuration methods Attempt to read espflash config from project_dir/.config/espflash.toml
5 participants
@SergioGasquez @MabezDev @bjoernQ @jessebraham @JurajSadel